c/snap: add --format json to snap tasks - #17017
Merged
Merged
Conversation
github-actions
Bot
added
the
Needs Documentation -auto-
|
Mon May 4 22:13:11 UTC 2026 No spread failures reported |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17017 +/- ##
==========================================
+ Coverage 78.98% 79.07% +0.08%
==========================================
Files 1381 1379 -2
Lines 191967 192447 +480
Branches 2464 2464
==========================================
+ Hits 151630 152169 +539
+ Misses 31184 31108 -76
- Partials 9153 9170 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
olivercalder
approved these changes
May 4, 2026
olivercalder
left a comment
olivercalder
left a comment
Member
There was a problem hiding this comment.
Small nitpicks, otherwise LGTM!
| func() flags.Commander { return &cmdTasks{} }, | ||
| changeIDMixinOptDesc.also(timeDescs), | ||
| changeIDMixinOptDesc.also(timeDescs).also(map[string]string{ | ||
| "format": i18n.G("Output format (json)."), |
Member
There was a problem hiding this comment.
Not sure if there's a spec for this, but maybe it could be this:
Suggested change
| "format": i18n.G("Output format (json)."), | |
| "format": i18n.G("Output format (supported: json)."), |
Contributor
Author
There was a problem hiding this comment.
--format wasn't originally in the spec, snapctl tasks adopted this to maintain compatiblity with Pebble, and snap tasks got it to reflect snapctl. Updated to match your suggestion and will ensure snapctl reflects this
| # Check that valid JSON output is produced for the change, and that it contains the expected task with the expected state. | ||
| echo "Check that 'snap tasks' lists the task for the change (and accepts the --format option)" | ||
| snap tasks --format json "$CHANGE_ID_1" | jq -e '.[0].id == "remove+comp1" and .[0].state == "done"' > /dev/null | ||
|
No newline at end of file |
Member
There was a problem hiding this comment.
Missing \n terminator or something? Not sure
Rnfudge02
force-pushed
the
snap-tasks
branch
2 times, most recently
from
May 5, 2026 13:43
c3f3006 to
ba9d3d1
Compare
|
Tue Jun 2 15:15:51 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
|
|
||
| # Check that valid JSON output is produced for the change, and that it contains the expected task with the expected state. | ||
| echo "Check that 'snap tasks' lists the task for the change (and accepts the --format option)" | ||
| snap tasks --format json "$CHANGE_ID_1" | jq -e '.[0].id == "remove+comp1" and .[0].state == "done"' > /dev/null |
Member
There was a problem hiding this comment.
I don't think that this check will pass as written right now, tasks isn't an array.
Contributor
Author
|
needs rebase |
andrewphelpsj
left a comment
andrewphelpsj
left a comment
Member
There was a problem hiding this comment.
Thanks, I think your spread test needs some changes. Otherwise, this is close. But I do think it'd be good to assess doing some copy-pasting of that formatMixin.
|
|
||
| execute: | | ||
| echo "Install component to trigger a change" | ||
| snap install test-snap-with-components+comp1 |
Member
There was a problem hiding this comment.
A couple things:
comp1isn't a component for this snap, it seems- We should probably just create a change for an installation from file, as to not depend on the store when we don't need to.
In this case, probably don't even need to use a component in this test.
|
|
||
| # Check that valid JSON output is produced for the change, and that it contains the expected task with the expected state. | ||
| echo "Check that 'snap tasks' lists the task for the change (and accepts the --format option)" | ||
| snap tasks --format json "$CHANGE_ID_1" | jq -e '.tasks | any(.id == "remove+comp1" and (.status == "Done" or .status == "done" or .state == "done"))' > /dev/null |
Member
There was a problem hiding this comment.
.id == "remove+comp1" isn't correct, I don't think. Task IDs are numbers.
Comment on lines
+11
to
+13
| restore: | | ||
| snap remove --purge test-snap-tasks || true | ||
|
|
Member
There was a problem hiding this comment.
Suggested change
| restore: | | |
| snap remove --purge test-snap-tasks || true |
| return err | ||
| } | ||
|
|
||
| if c.Format == "json" { |
Member
There was a problem hiding this comment.
I wonder if it's better to just take the formatMixin from pebble, then it'll be easier to stay aligned with them.
…or to 2.76 (canonical#17076)" This reverts commit 40008b5.
…anch covers this feature
andrewphelpsj
approved these changes
May 28, 2026
andrewphelpsj
left a comment
andrewphelpsj
left a comment
Member
There was a problem hiding this comment.
Thanks! LGTM, but I expect that your spread test isn't passing right now. You'll need to address that before merging.
|
|
||
| # Check that valid JSON output is produced for the change, and that it contains the expected task with the expected state. | ||
| echo "Check that 'snap tasks' lists the task for the change (and accepts the --format option)" | ||
| snap tasks --format json "$CHANGE_ID" | jq -e '.tasks | any(.id == "$CHANGE_ID" and (.status == "Done" or .status == "done" or .state == "done"))' > /dev/null |
Member
There was a problem hiding this comment.
This jq assertion won't work because $CHANGE_ID won't expand, as the expression is inside of single quotes.
Member
There was a problem hiding this comment.
Also, please use gojq like the other spread tests.
| c.Assert(err, check.ErrorMatches, ".*Invalid value `' for option `--format'. Allowed values are: .* or json") | ||
| _, err = snap.Parser(snap.Client()).ParseArgs([]string{"tasks", "--format=random", "42"}) | ||
| c.Assert(err, check.ErrorMatches, ".*Invalid value `random' for option `--format'. Allowed values are: .* or json") | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is the final of the async snapctl work. This PR makes
snap tasks --format jsonso that thesnap taskscommand aligns with thesnapctl taskscommand. Only the last commit of this PR are the changes made, the first 8 belong to themove_daemonbranch.